Skip to content

Conversation

@robertbartel
Copy link
Contributor

@robertbartel robertbartel commented Oct 29, 2025

The existing BMI adapter for Fortran modules will not behave correctly in situations when the size of Fortran types is modified from the "standard" expected size. This is because the adapter does not ask the module directly for variable item sizes when deciding what corresponding type to use for values inside the framework.

For example, consider this method from Bmi_Fortran_Adapter:

            inline void inner_get_value(const std::string& name, void *dest) {
                std::string varType = inner_get_var_type(name);
                //Can use the C type or the fortran type, e.g. int or integer
                if (varType == "int" || varType == "integer") {
                    inner_get_value_int(name, (int *)dest);
                }
                else if (varType == "float" || varType == "real") {
                    inner_get_value_float(name, (float *)dest);
                }
                else if (varType == "double" || varType == "double precision") {
                    inner_get_value_double(name, (double *)dest);
                }
                else {
                    throw ::external::ExternalIntegrationException(
                            "Can't get model " + model_name + " variable " + name + " of type '" + varType + ".");
                }
            }

The adapter only asks for a variable's type and assumes the appropriate item size strictly based on the type name string: e.g., that an advertised integer will be the same size as a C++ int. This is not guaranteed to be correct, as type resizing can be done at compile time, using flags like gfortran's -fdefault-real-8.

Changes

  • Correct get_analogous_cxx_type to get the C++ type corresponding to not just the category (i.e., integer, float) but also the memory size of a variable
  • Correct adapter BMI getter/setter functions similarly, having them now defer to get_analogous_cxx_type for determining how types should be handled
  • Moving the first level of getter/setter logic from "inner" methods to directly within the main BMI GetValue/SetValue implementations, since updated implementations now violated the intended premise of "inner" functions for the class.

Testing

  1. Existing unit tests pass as they did previously (actually, they are ... there are a few failing (though seemingly unrelated) tests ... I'm investigating)

Notes

  • I removed some "inner" functions (see the header file for details), because doing so fell within the scope. The necessary updates violated the stated premise for "inner" functions, thus imposing a need to move where the logic was placed (and thereby making redundant the functions that were removed). The class would likely benefit from refactoring to remove other "inner" functions also, and just have logic be directly within the implemented BMI functions. However, that would be beyond the scope for this.

Todos

  • Before taking out of draft status, I need to confirm the functionality by re-compiling the Fortran components locally with the adjusted precision, and then running a few tests.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • Linux

Fixing get_analogous_cxx_type and correcting getter/setter functions to
properly consider both the type name string and the item size of a BMI
module variable in handling the analogous internal typing, since Fortran
type sizes (and thus the required analogous C++ type) can be changed by
compiler settings.

Also, since corrected getter/setter implementation relied on nested
calls to non-inner get_analogous_cxx_type method, moving the
getter/setter logic to be directly within GetValue/SetValue methods.
@robertbartel robertbartel added bug Something isn't working QA/QC labels Oct 29, 2025
@PhilMiller
Copy link
Contributor

This sounds like a nice change. Did the case(s) in question come up with one of the BMI models we have at hand?

@robertbartel
Copy link
Contributor Author

This sounds like a nice change. Did the case(s) in question come up with one of the BMI models we have at hand?

Indirectly. As part of the SacSMA mass balance issue investigation, one of the things that was tried was changing the precision of reals to see if it impacted the imbalance. This didn't go super smoothly at first, and as a result I end up looking at the adapter code and realizing it was making those assumptions about type sizing (in a way that, for example, the Python BMI adapter wasn't).

That said, it may be the case that ISO_C_BINDING middleware also doesn't play nicely with such precision changes ... I'm still doing some testing. But accounting for variable type size changes at the adapter level may turn out to be moot unless the middleware is also updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working QA/QC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants